Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

fix(schema): preserve top-level undefined keys in runtime config #4459

Closed
wants to merge 1 commit into from

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Apr 20, 2022

πŸ”— Linked issue

nuxt/nuxt#13769

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This is a behaviour of defu that we have previously resolved in unjs/nitro#28 and #2456, but which has regressed...

It will also need to be paired with a PR to nitro and c12.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added bug Something isn't working schema πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage labels Apr 20, 2022
@danielroe danielroe requested a review from pi0 April 20, 2022 13:28
@danielroe danielroe self-assigned this Apr 20, 2022
@netlify
Copy link

netlify bot commented Apr 20, 2022

βœ… Deploy Preview for nuxt3-docs ready!

Name Link
πŸ”¨ Latest commit 963ea58
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/62600a6ce98c600009187918
😎 Deploy Preview https://deploy-preview-4459--nuxt3-docs.netlify.app
πŸ“± Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -720,14 +720,19 @@ export default {
* @version 3
*/
runtimeConfig: {
$resolve: (val: RuntimeConfig, get) => defu(val, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you e2e tested this? c12 and nitro use default multiple times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we have a chance to map all NITRO_ and NUXT_ values into runtime config without explicit definition.

Copy link
Member Author

@danielroe danielroe Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will need PRs to c12 & nitro. Issue is only at top-level IIRC so nested values with undefined are fine.

Tricky thing about automatic mapping is that it doesn't work both ways (ie. given a value in the config object, we can find the env variable name, but not vice versa).

For example, NUXT_BASE_URL maps to all of the following:

{
  base: {
    url: ''
  },
  baseURL: '',
  baseUrl: '',
  base_url: '',
  BaseUrl: '',
  // etc.
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can alternatively require fallback values in runtimeConfig?

Copy link
Member

@pi0 pi0 Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make a convention work as top-level camelCase. using undefined as default is not a solution I would keep forever... But let's merge it in last minutes. We can document to use empty string also for fallbacks instead of undefined.

@pi0 pi0 closed this Apr 20, 2022
@pi0 pi0 deleted the fix/schema-runtime-config branch April 20, 2022 14:12
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3.x bug Something isn't working πŸ”¨ p3-minor-bug Priority 3: a bug in an edge case that only affects very specific usage schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants